[Improvement] Defer sending doc changes to the agent to an idle timer. #176
Conversation
raymond-w-ko
force-pushed
the
fix-gh-172
branch
from
b738349
to
a046810
Compare
I am not an Emacs expert, but my hypothesis is that jsonrpc-notify causes buffer navigation to occur, which causes the (org-todo) function to break when called twice in a row. This fixes issue: copilot-emacs#172
also refactor out some variables so lines are not as long, and add a commented out debug message for future use.
This is used in case copilot stops responding due to buffer state being desynchronized with the copilot process. It is unsure whether this is still necessary after the previous commits.
raymond-w-ko
force-pushed
the
fix-gh-172
branch
from
a046810
to
a0a685f
Compare
|
The original pull request would never have worked properly, since I thought There is also some other extra stuff here, like the But assuming this is the wrong way to fix #172, there isn't anything too important here to merge. |
@raymond-w-ko I am confused. does this PR fix the linked issue or not? |
|
I will push changes and get this merged. |
There was a problem hiding this comment.
LGTM. :)
Some nits that are good to resolve:
In toplevel form:
copilot.el:39:35: Warning: Alias for ‘copilot-completion-idle-delay’ should be declared before its referent
In copilot-diagnose:
copilot.el:311:7: Warning: reference to free variable ‘copilot-mode’
In copilot--handle-notification:
copilot.el:542:14: Warning: ‘mark-whole-buffer’ is for interactive use only.
In copilot--on-doc-change:
copilot.el:665:32: Warning: reference to free variable ‘copilot-mode’
In copilot--on-doc-focus:
copilot.el:675:14: Warning: reference to free variable ‘copilot-mode’
In copilot-async-start-process:
copilot.el:[105](https://github.com/copilot-emacs/copilot.el/actions/runs/8222298553/job/22483626619?pr=176#step:5:106)0:10: Warning: Unused lexical variable ‘name’
In end of data:
copilot.el:804:14: Warning: the function ‘vterm-insert’ is not known to be defined.
copilot.el:803:14: Warning: the function ‘vterm-delete-region’ is not known to be defined.
copilot.el:543:14: Warning: the function ‘org-sort-entries’ is not known to be defined.
copilot.el:532:54: Warning: the function ‘org-entry-get’ is not known to be defined.
copilot.el:532:26: Warning: the function ‘org-map-entries’ is not known to be defined.
But we can fix these warnings later.
|
One last thing to check is that |
| @@ -26,7 +26,7 @@ | |||
| :group 'completion | |||
| :prefix "copilot-") | |||
|
|
|||
| (defcustom copilot-idle-delay 0 | |||
| (defcustom copilot-completion-idle-delay 0 | |||
There was a problem hiding this comment.
what's the motivation for this change?
Personally, I think copilot-idle-delay is fine and probably doesn't warrant the complexity of adding a deprecated variable.
| ;; | ||
| ;; Doc changes | ||
| ;; | ||
| (cl-defmacro copilot--widening (&rest body) |
There was a problem hiding this comment.
It feels like this change doens't quite fit the PR description? Separate PR? Not clear to me why it's necessary since the point should be within the narrowed region, right?
There was a problem hiding this comment.
Change appears to not create any regressions, however, I'm unable to reproduce the original issue on master, which causes me to wonder if we really understand what is broken?
I have concerns about the widen change. Why is this introduced with this PR? There's no explanation as to what motivated it, and it appears unrelated and also I cannot understand why it is needed (copilot operates right after the point, and the point should always be in a narrowed region? In my experiments, I have no trouble using copilot within a narrowed region. A separate issue should be filed.
In my recordings, I'm using Emacs 29.2, with the built-in version of org-mode, and a minimal environment (package-initialize, keycast-mode enabled, copilot-mode enabled (from source at commit 0d48e75)
Given that I can't reproduce the original issue on master, I'm wondering if this change is needed
|
@timcharper Although I did manage to reproduce the problem initially I am also not seeing it anymore on Emacs I do, however, feel like this is a good improvement nonetheless. That being deferring jsonrpc communication with the server to some idle time or until the user requests a completion (as implemented in and shamelessly stolen from In any event, the PR title and description needs some work. I will update. |
emil-vdw
changed the title
Currently, doc changes (like
textDocument/didChange) are communicated on every change (essentially every key press). It would make more sense and improve responsiveness to defer communicating with the server until some idle time or until it is actually needed for a completion.To implement this we don't have to reinvent the wheel and can just import most of it from the
eglotimplementation.Implementation
This fixes issue:
#172 (although this is also seems to not be an issue in newer
orgversions).